Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Applying new style to entire selection if partially present #1056

Merged
merged 6 commits into from
Nov 20, 2017
Merged

Conversation

beatadelura
Copy link
Contributor

@beatadelura beatadelura commented Oct 16, 2017

What is the purpose of this pull request?

Bug fix

Does your PR contain necessary tests?

Yes

This PR contains

  • Unit tests
  • Manual tests

Closes #1040

@f1ames f1ames self-requested a review November 2, 2017 16:02
f1ames
f1ames previously requested changes Nov 3, 2017
},

// #1040
'test apply new style to entire selection if partially present': function() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more test checking if styles are not changed in any way if applied second time on the part of already styled element could be added.

// #1040
'test apply new style to entire selection if partially present': function() {
var bot = this.editorBot;
bender.tools.selection.setWithHtml( bot.editor, '<h1><span style="font-family:Courier New,Courier,monospace">[Hello</span> world!]</h1>' );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ffCourierNew could be also used here.

bot.combo( 'Font', function( combo ) {
combo.onClick( 'Courier New' );
this.wait( function() {
combo.onClick( 'Courier New' );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure why in this test combo is clicked two times? One should be enough to cover this case.

@@ -0,0 +1,14 @@
@bender-tags: 4.8.0, bug, 1040
@bender-ui: collapsed
@bender-ckeditor-plugins: wysiwygarea, toolbar, font
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The elementspath plugin could be useful, so tester could see if any strange markup wasn't created.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And a sourcearea plugin.

@bender-ui: collapsed
@bender-ckeditor-plugins: wysiwygarea, toolbar, font

**Scenario:**
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would add a second scenario with font size and different selection (which will contain styles at the end, not at the beginning - so first changing size of world text and then selecting He{llo world} and reapplying the styles).

@f1ames
Copy link
Contributor

f1ames commented Nov 3, 2017

Overtaking this PR.

@f1ames f1ames force-pushed the t/1040 branch 2 times, most recently from a1fa080 to d5f566e Compare November 3, 2017 10:21
@Comandeer Comandeer dismissed f1ames’s stale review November 20, 2017 17:04

Newer review is available.

@Comandeer Comandeer merged commit 6b957d9 into major Nov 20, 2017
@Comandeer Comandeer deleted the t/1040 branch November 20, 2017 17:05
@mlewand mlewand changed the title T/1040 Applying new style to entire selection if partially present Applying new style to entire selection if partially present Jan 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New style not applied to entire selection if partially present
3 participants